Skip to content

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Aug 18, 2024

This is my attempt to un-stall #63569 and #79995, by creating methods that mirror the existing MaybeUninit API:

impl<T> MaybeUninit<T> {
    pub fn write(&mut self, value: T) -> &mut T;
    pub fn as_bytes(&self) -> &[MaybeUninit<u8>];
    pub fn as_bytes_mut(&mut self) -> &mut [MaybeUninit<u8>];
    pub unsafe fn assume_init_drop(&mut self);
    pub unsafe fn assume_init_ref(&self) -> &T;
    pub unsafe fn assume_init_mut(&mut self) -> &mut T;
}

Adding these APIs:

impl<T> [MaybeUninit<T>] {
    // replacing copy_from_slice; renamed to avoid conflict
    pub fn write_copy_of_slice(&mut self, value: &[T]) -> &mut [T] where T: Copy;

    // replacing clone_from_slice; renamed to avoid conflict
    pub fn write_clone_of_slice(&mut self, value: &[T]) -> &mut [T] where T: Clone;

    // identical to non-slice versions; no conflict
    pub fn as_bytes(&self) -> &[MaybeUninit<u8>];
    pub fn as_bytes_mut(&mut self) -> &mut [MaybeUninit<u8>];
    pub unsafe fn assume_init_drop(&mut self);
    pub unsafe fn assume_init_ref(&self) -> &[T];
    pub unsafe fn assume_init_mut(&mut self) -> &mut [T];
}

Since the assume_init methods are identical to those on non-slices, they feel pretty natural. The main issue with the write methods is naming, as discussed in #79995 among other places. My rationale:

  • The term "write" should be in them somewhere, to mirror the other API, and this pretty much automatically makes them not collide with any other inherent slice methods.
  • I chose write_clone_of_slice and write_copy_of_slice since clone and copy are being used as objects here, whereas they're being used as actions in clone_from_slice and copy_from_slice.

The final "weird" thing I've done in this PR is remove a link to Vec<T> from assume_init_drop (both copies, since they're effectively copied docs), since there's no good way to link to Vec for something that can occur both on the page for std/primitive.slice.html and std/vec/struct.Vec.html, since the code here lives in libcore and can't use intra-doc-linking to mention Vec. (see: #121436)

The reason why this method shows up both on Vec<T> and [T] is because the [T] docs are automatically inlined on Vec<T>'s page, since it implements Deref. It's unfortunate that rustdoc doesn't have a way of dealing with this at the moment, but it is what it is, and it's a reasonable compromise for now.

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 18, 2024
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor

The implementation looks pretty reasonable to me, but as this is an API decision:

r? libs-api

Is the intent here to replace the existing copy_from_slice and clone_from_slice? If so, those should be removed.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 19, 2024
@rustbot rustbot assigned BurntSushi and unassigned tgross35 Aug 19, 2024
@clarfonthey
Copy link
Contributor Author

I decided to not delete them since so many people are using these methods, but I can deprecate them depending on what people think is best.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the maybe_uninit_slices branch 2 times, most recently from 0cd81e0 to 6641ecd Compare September 1, 2024 19:56
@rust-log-analyzer

This comment has been minimized.

@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 20, 2024
@Amanieu
Copy link
Member

Amanieu commented Nov 26, 2024

We discussed this in the libs-api meeting. We're happy with these changes, but we would also like to see similar changes for the fill (#117428) and as_bytes (#93092) methods so that all slice-related methods are turned into inherent methods. Note that fill will need to be renamed since it conflicts with the generic fill method on slices (perhaps fill_init?).

Also, the old methods on MaybeUninit should be removed (in a separate PR) so that we don't have duplicate methods that do the same thing.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 26, 2024
@tgross35 tgross35 self-assigned this Dec 19, 2024
@tgross35
Copy link
Contributor

Based on the above it seems like there are a few updates needed here.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

Figured it makes more sense to pong this back to libs-api than mark it as my review, since libs-api did want to include these methods in the final PR.

r? libs-api
@rustbot ready

@rustbot rustbot assigned joshtriplett and unassigned tgross35 Jan 2, 2025
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 2, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

@clarfonthey any chance you are willing to reconsider moving fill changes to another PR? I don't think there is any reason that change needs to happen at the same time.

Just asking because I'd like to start using this new API :)

@clarfonthey
Copy link
Contributor Author

I mean, if you're cool merging this as-is without the fill methods, I'm fine splitting it. Just want to clarify which methods should be left in and which are left out.

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

Specifically the changes fill -> write_filled, fill_with -> write_with, fill_from -> write_iter, since the names are the only things not okayed by someone from libs-api at #129259 (comment). So everything except those three are 👍 from me.

@clarfonthey
Copy link
Contributor Author

Rebased and locally tested, but will wait for CI to pass before calling this officially ready. Moved the second half of the PR to #135394.

@clarfonthey
Copy link
Contributor Author

Should be ready to merge!

@tgross35
Copy link
Contributor

🎉

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 12, 2025

📌 Commit e37daf0 has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#129259 (Add inherent versions of MaybeUninit methods for slices)
 - rust-lang#135374 (Suggest typo fix when trait path expression is typo'ed)
 - rust-lang#135377 (Make MIR cleanup for functions with impossible predicates into a real MIR pass)
 - rust-lang#135378 (Remove a bunch of diagnostic stashing that doesn't do anything)
 - rust-lang#135397 (compiletest: add erroneous variant to `string_enum`s conversions error)
 - rust-lang#135398 (add more crash tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 08968a4 into rust-lang:master Jan 12, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 12, 2025
@clarfonthey clarfonthey deleted the maybe_uninit_slices branch January 12, 2025 17:27
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2025
Rollup merge of rust-lang#129259 - clarfonthey:maybe_uninit_slices, r=tgross35

Add inherent versions of MaybeUninit methods for slices

This is my attempt to un-stall rust-lang#63569 and rust-lang#79995, by creating methods that mirror the existing `MaybeUninit` API:

```rust
impl<T> MaybeUninit<T> {
    pub fn write(&mut self, value: T) -> &mut T;
    pub fn as_bytes(&self) -> &[MaybeUninit<u8>];
    pub fn as_bytes_mut(&mut self) -> &mut [MaybeUninit<u8>];
    pub unsafe fn assume_init_drop(&mut self);
    pub unsafe fn assume_init_ref(&self) -> &T;
    pub unsafe fn assume_init_mut(&mut self) -> &mut T;
}
```

Adding these APIs:

```rust
impl<T> [MaybeUninit<T>] {
    // replacing copy_from_slice; renamed to avoid conflict
    pub fn write_copy_of_slice(&mut self, value: &[T]) -> &mut [T] where T: Copy;

    // replacing clone_from_slice; renamed to avoid conflict
    pub fn write_clone_of_slice(&mut self, value: &[T]) -> &mut [T] where T: Clone;

    // identical to non-slice versions; no conflict
    pub fn as_bytes(&self) -> &[MaybeUninit<u8>];
    pub fn as_bytes_mut(&mut self) -> &mut [MaybeUninit<u8>];
    pub unsafe fn assume_init_drop(&mut self);
    pub unsafe fn assume_init_ref(&self) -> &[T];
    pub unsafe fn assume_init_mut(&mut self) -> &mut [T];
}
```

Since the `assume_init` methods are identical to those on non-slices, they feel pretty natural. The main issue with the write methods is naming, as discussed in rust-lang#79995 among other places. My rationale:

* The term "write" should be in them somewhere, to mirror the other API, and this pretty much automatically makes them not collide with any other inherent slice methods.
* I chose `write_clone_of_slice` and `write_copy_of_slice` since `clone` and `copy` are being used as objects here, whereas they're being used as actions in `clone_from_slice` and `copy_from_slice`.

The final "weird" thing I've done in this PR is remove a link to `Vec<T>` from `assume_init_drop` (both copies, since they're effectively copied docs), since there's no good way to link to `Vec` for something that can occur both on the page for `std/primitive.slice.html` and `std/vec/struct.Vec.html`, since the code here lives in libcore and can't use intra-doc-linking to mention `Vec`. (see: rust-lang#121436)

The reason why this method shows up both on `Vec<T>` and `[T]` is because the `[T]` docs are automatically inlined on `Vec<T>`'s page, since it implements `Deref`. It's unfortunate that rustdoc doesn't have a way of dealing with this at the moment, but it is what it is, and it's a reasonable compromise for now.
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#129259 (Add inherent versions of MaybeUninit methods for slices)
 - rust-lang#135374 (Suggest typo fix when trait path expression is typo'ed)
 - rust-lang#135377 (Make MIR cleanup for functions with impossible predicates into a real MIR pass)
 - rust-lang#135378 (Remove a bunch of diagnostic stashing that doesn't do anything)
 - rust-lang#135397 (compiletest: add erroneous variant to `string_enum`s conversions error)
 - rust-lang#135398 (add more crash tests)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 19, 2025
…r=tgross35

`MaybeUninit` inherent slice methods part 2

These were moved out of rust-lang#129259 since they require additional libs-api approval. Tracking issue: rust-lang#117428.

New API surface:

```rust
impl<T> [MaybeUninit<T>] {
    // replacing fill; renamed to avoid conflict
    pub fn write_filled(&mut self, value: T) -> &mut [T] where T: Clone;

    // replacing fill_with; renamed to avoid conflict
    pub fn write_with<F>(&mut self, value: F) -> &mut [T] where F: FnMut() -> T;

    // renamed to remove "fill" terminology, since this is closer to the write_*_of_slice methods
    pub fn write_iter<I>(&mut self, iter: I) -> (&mut [T], &mut Self) where I: Iterator<Item = T>;
}
```

Relevant motivation for these methods; see rust-lang#129259 for earlier methods' motiviations.

* I chose `write_filled` since `filled` is being used as an object here, whereas it's being used as an action in `fill`.
* I chose `write_with` instead of `write_filled_with` since it's shorter and still matches well.
* I chose `write_iter` because it feels completely different from the fill methods, and still has the intent clear.

In all of the methods, it felt appropriate to ensure that they contained `write` to clarify that they are effectively just special ways of doing `MaybeUninit::write` for each element of a slice.

Tracking issue: rust-lang#117428

r? libs-api
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2025
Rollup merge of rust-lang#135394 - clarfonthey:uninit-slices-part-2, r=tgross35

`MaybeUninit` inherent slice methods part 2

These were moved out of rust-lang#129259 since they require additional libs-api approval. Tracking issue: rust-lang#117428.

New API surface:

```rust
impl<T> [MaybeUninit<T>] {
    // replacing fill; renamed to avoid conflict
    pub fn write_filled(&mut self, value: T) -> &mut [T] where T: Clone;

    // replacing fill_with; renamed to avoid conflict
    pub fn write_with<F>(&mut self, value: F) -> &mut [T] where F: FnMut() -> T;

    // renamed to remove "fill" terminology, since this is closer to the write_*_of_slice methods
    pub fn write_iter<I>(&mut self, iter: I) -> (&mut [T], &mut Self) where I: Iterator<Item = T>;
}
```

Relevant motivation for these methods; see rust-lang#129259 for earlier methods' motiviations.

* I chose `write_filled` since `filled` is being used as an object here, whereas it's being used as an action in `fill`.
* I chose `write_with` instead of `write_filled_with` since it's shorter and still matches well.
* I chose `write_iter` because it feels completely different from the fill methods, and still has the intent clear.

In all of the methods, it felt appropriate to ensure that they contained `write` to clarify that they are effectively just special ways of doing `MaybeUninit::write` for each element of a slice.

Tracking issue: rust-lang#117428

r? libs-api
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 2, 2025
…r=tgross35

`MaybeUninit` inherent slice methods part 2

These were moved out of rust-lang#129259 since they require additional libs-api approval. Tracking issue: rust-lang#117428.

New API surface:

```rust
impl<T> [MaybeUninit<T>] {
    // replacing fill; renamed to avoid conflict
    pub fn write_filled(&mut self, value: T) -> &mut [T] where T: Clone;

    // replacing fill_with; renamed to avoid conflict
    pub fn write_with<F>(&mut self, value: F) -> &mut [T] where F: FnMut() -> T;

    // renamed to remove "fill" terminology, since this is closer to the write_*_of_slice methods
    pub fn write_iter<I>(&mut self, iter: I) -> (&mut [T], &mut Self) where I: Iterator<Item = T>;
}
```

Relevant motivation for these methods; see rust-lang#129259 for earlier methods' motiviations.

* I chose `write_filled` since `filled` is being used as an object here, whereas it's being used as an action in `fill`.
* I chose `write_with` instead of `write_filled_with` since it's shorter and still matches well.
* I chose `write_iter` because it feels completely different from the fill methods, and still has the intent clear.

In all of the methods, it felt appropriate to ensure that they contained `write` to clarify that they are effectively just special ways of doing `MaybeUninit::write` for each element of a slice.

Tracking issue: rust-lang#117428

r? libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants